Skip to content

fix(kafka): resolve disk-removal deadlock during rolling upgrade - #254

Merged
dobrerazvan merged 7 commits into
masterfrom
fix-disk-removal-deadlock-during-rolling-upgrade
Aug 26, 2026
Merged

fix(kafka): resolve disk-removal deadlock during rolling upgrade#254
dobrerazvan merged 7 commits into
masterfrom
fix-disk-removal-deadlock-during-rolling-upgrade

Conversation

@dobrerazvan

@dobrerazvan dobrerazvan commented May 18, 2026

Copy link
Copy Markdown

Problem

Two related deadlocks left clusters stuck in ClusterRollingUpgrading indefinitely, because reconcileKafkaPvc aborts the entire reconcile with CruiseControlTaskRunning ("Disk removal pending") before reconcileKafkaPod — which creates/restarts pods — ever runs.

  1. Missing pod + pending removal (original report): a broker pod deleted during a rolling upgrade while a disk removal is pending is never recreated. Cruise Control can't finish the removal (it needs the broker running), and the block prevents the pod from coming back. Observed in production: broker 103 of a 9-broker cluster deleted and never recreated, looping every ~20s for 10+ minutes.
  2. Stalled removal + pods present (found via a second live incident): with all pods up but the CC removal task terminally failed, the reconcile stays frozen forever. GracefulDisk{Removal,Rebalance}CompletedWithError / Paused are bucketed under IsDiskRemovalRunning() (common_types.go:83), so a failed task is treated as in-progress and blocks config rollout to healthy brokers. Observed on a 3-broker cluster with every removed disk in GracefulDiskRemovalCompletedWithError, ConfigOutOfSync brokers unable to restart.

Fix

(1) Missing-pod bypass — build runningBrokers before reconcileKafkaPvc and pass it in; before returning the blocking error, if a broker with pending removal/rebalance has no pod, return nil so the pod can be recreated.

(2) Narrow the blocking wait — add CruiseControlVolumeState.IsDiskOperationStalled() (CompletedWithError / Paused, mirroring IsDownscaleStalled) and, in handleDiskRemoval, set waitForDiskRemovalToFinish only for genuinely-progressing states. Branch selection (PVC deletion, state marking) is unchanged.

The two are complementary.

Why relaxing the block is data-safe

The block is not what protects log.dirs integrity — two independent mechanisms already do, both keyed off success, not the desired spec:

  • log.dirs retention (shouldKeepRemovedLogDirInConfig, configmap.go:312-335): the removed disk stays in log.dirs until the state is …Succeeded; the KRaft path's shrink (configmap.go:202) is overwritten by the protective merge (configmap.go:90-97).
  • PVC mount retention (pod.go:438, getCreatedPvcForBroker): the pod mounts all existing PVCs; the removed disk's PVC is deleted only on IsDiskRemovalSucceeded().

So a ConfigOutOfSync broker can restart mid-removal with the disk still in log.dirs and mounted — no stranded data.

Verified against Cruise Control source: remove_disks runs as intra-broker replica movement; a dead broker causes CC to mark the task DEAD ("destination disk is down", Executor.java:2145-2151) and complete — it does not hang in execution — so a stalled state is genuinely terminal, not in-flight work a restart could disrupt.

Testing

  • go build ./..., go vet, and unit tests pass in both modules (main + api/v1beta1).
  • New TestReconcileKafkaPvcDiskRemoval cases: removal CompletedWithError, removal Paused, and rebalance CompletedWithError with pod present now proceed; Running + pod present still blocks (regression guard); existing missing-pod cases unchanged.

Not in scope

Driving a CompletedWithError removal to actually retry/succeed (vs. just no longer blocking) — a follow-up in the CC task reconciler.

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change
  • Refactor
  • Documentation
  • Other (please describe)

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

dobrerazvan and others added 3 commits August 24, 2026 10:34
When a broker pod is deleted during rolling upgrade and a disk removal is
pending (GracefulDiskRemovalScheduled), the operator enters a deadlock:
reconcileKafkaPvc blocks the entire reconcile with "Disk removal pending",
preventing reconcileKafkaPod from recreating the missing pod. Meanwhile,
Cruise Control cannot complete the disk removal because the broker isn't
running.

Fix: move runningBrokers map building before reconcileKafkaPvc and pass it
in. Before returning the blocking error, check if any broker with pending
disk removal has a missing pod. If so, allow the reconcile to proceed so
the pod can be recreated. The disk removal check is re-evaluated on the
next cycle once the broker is back up.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… add tests

- Fix #1 (HIGH): Override now checks IsDiskRebalance() in addition to
  IsDiskRemoval(), closing the same deadlock vector for rebalance states
- Fix #2 (LOW): Include mountPath in the bypass log message for
  consistency with other disk-removal log messages
- Fix #3 (LOW): Add tests for rebalance-state deadlock bypass and for
  newly-marked-for-removal with missing pod

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Proposal, design, and task tracking for the fix.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amuraru
amuraru force-pushed the fix-disk-removal-deadlock-during-rolling-upgrade branch from c0c20d0 to e21f0ab Compare August 24, 2026 08:34
@eduardagarici

Copy link
Copy Markdown

Should we add an e2e test to simulate full PVC reconciliation at step N, N+1, in this pod missing condition?

A terminally-failed or paused Cruise Control disk operation
(GracefulDisk{Removal,Rebalance}CompletedWithError / Paused) was treated
as "in progress" via IsDiskRemovalRunning(), so reconcileKafkaPvc returned
CruiseControlTaskRunning indefinitely. With all broker pods present this
froze the whole reconcile — including config rollout / rolling upgrade to
healthy brokers — since reconcileKafkaPod runs downstream of the PVC block.
Observed on a live 3-broker cluster stuck in ClusterRollingUpgrading with
every removed disk in GracefulDiskRemovalCompletedWithError.

Relaxing the block is data-safe: log.dirs retention
(shouldKeepRemovedLogDirInConfig) and PVC mount retention both keep the
removed disk in place until removal is confirmed *succeeded*, independent
of this block. Cruise Control also does not hang on a dead broker — it
marks the intra-broker task DEAD and completes with error — so a stalled
state is genuinely terminal, not in-flight work a restart could disrupt.

- add CruiseControlVolumeState.IsDiskOperationStalled() (CompletedWithError
  /Paused), mirroring IsDownscaleStalled
- in handleDiskRemoval, set waitForDiskRemovalToFinish only for non-stalled
  IsDiskRemoval()/IsDiskRebalance() states; branch selection (PVC deletion,
  state marking) is unchanged
- tests: removal CompletedWithError / Paused and rebalance CompletedWithError
  with pod present now proceed; Running + pod present still blocks

Complements the existing missing-pod bypass: narrowing handles a stalled
task with pods up; the bypass handles a genuinely-running task whose broker
pod is missing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dobrerazvan dobrerazvan changed the title Fix disk removal deadlock during rolling upgrade fix(kafka): resolve disk-removal deadlock during rolling upgrade Aug 26, 2026
Add an e2e spec that applies a single manifest carrying both a read-only
broker config change (log.retention.hours, forces a rolling restart) and a
disk removal, then asserts the cluster reconciles correctly: the config
change propagates to broker ConfigMaps, the removed disk drops out of
log.dirs, Cruise Control goes quiescent, and the cluster returns to
ClusterRunning (a deadlock would time out this wait).

- config/samples/simplekafkacluster_1disk_configchange.yaml: 2disk sample
  reduced to one disk (removes /kafka-logs3) plus a changed readOnlyConfig
- tests/e2e/test_config_change_with_disk_removal.go: testConfigChangeWithDiskRemoval
  and brokerConfigMapsContainProperty helper
- wire the spec into the suite after testMultiDiskRemoval (chains 2->1 disk)

Covers the combined-operation happy path end to end; the stalled-removal and
missing-pod deadlocks the fix targets remain covered by the unit tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dobrerazvan

Copy link
Copy Markdown
Author

test(e2e): verify config change + disk removal reconcile together

added in 5ffc741

The added stalled-state cases pushed the table-driven test past the funlen
limit (355 > 323). Annotate with //nolint:funlen, matching the existing
convention for long table tests in this file.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dobrerazvan
dobrerazvan merged commit 05fb765 into master Aug 26, 2026
7 checks passed
@dobrerazvan
dobrerazvan deleted the fix-disk-removal-deadlock-during-rolling-upgrade branch August 26, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants